Skip to content

feat(runtime): add auto-stop for max iterations in non-interactive mode#2208

Open
tdabasinskas wants to merge 1 commit intodocker:mainfrom
cogvel:fix/auto-stop-max-iterations
Open

feat(runtime): add auto-stop for max iterations in non-interactive mode#2208
tdabasinskas wants to merge 1 commit intodocker:mainfrom
cogvel:fix/auto-stop-max-iterations

Conversation

@tdabasinskas
Copy link
Contributor

When max iterations are reached in non-interactive mode (e.g., MCP server), the runtime now automatically stops execution instead of blocking indefinitely waiting for user input. This prevents the system from hanging when ToolsApproved is true and provides a clear assistant message explaining why execution was stopped.

When max iterations are reached in non-interactive mode (e.g., MCP server), the runtime now automatically stops execution instead of blocking indefinitely waiting for user input. This prevents the system from hanging when `ToolsApproved` is true and provides a clear assistant message explaining why execution was stopped.
@tdabasinskas tdabasinskas requested a review from a team as a code owner March 21, 2026 17:52
@derekmisler
Copy link
Contributor

/review

Copy link

@docker-agent docker-agent bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🔴 CRITICAL

Found 1 critical issue in the changed code that needs attention.


// In non-interactive mode (e.g. MCP server), auto-stop instead of
// blocking forever waiting for user input.
if sess.ToolsApproved {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 HIGH SEVERITY: Auto-stop logic incorrectly triggers in interactive sessions with tools approved

The auto-stop logic checks only sess.ToolsApproved to determine non-interactive mode, but this flag is set in two distinct scenarios:

  1. MCP Server Mode (pkg/mcp/server.go:145): ToolsApproved is set to true for non-interactive MCP tool calls
  2. Interactive TUI Mode (pkg/cli/runner.go:119): ToolsApproved is set to true when users select "ConfirmationApproveSession" to approve all tools

The Problem:
In interactive TUI sessions where users have approved tools, this auto-stop will prevent them from manually resuming after max iterations. The stream exits with return, making it impossible for users to continue the conversation interactively as they would expect in TUI mode.

Recommendation:
The condition should distinguish between truly non-interactive contexts (MCP server, API mode) and interactive TUI sessions with tools approved. Consider:

  • Adding a dedicated sess.NonInteractive flag, or
  • Checking sess.ToolsApproved && !r.isInteractiveMode(), or
  • Using a different session property that specifically indicates MCP/API server mode

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, sounds valid. This flag indeed can also be set mid-session when a user selects "approve all tools" in the TUI, not just in non-interactive modes.

However, I think the practical impact is limited:

  • In the CLI/TUI, the MaxIterationsReachedEvent is emitted before this early return. The outer runner receives it and would attempt rt.Resume(), but the loop has already exited. The result is the stream closes cleanly - the user just can't extend iterations. This is a behavior change but not a hang or crash.

  • The current behavior without this patch is that MCP/A2A/eval sessions block forever on r.resumeChan since there's no consumer to call rt.Resume(). I believe this is the actual bug.

I guess to properly distinguish non-interactive contexts, we'd need something like a sess.NonInteractive runtime option set only in MCP server, A2A, and evaluation modes.

@derekmisler, what are your thoughts on this? Should I update the code to introduce sess.NonInteractive and use it instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great analysis from both sides here. The bot's review is valid — sess.ToolsApproved isn't a reliable proxy for non-interactive mode since it gets set to true in two distinct paths:

  1. Non-interactive (MCP, A2A, eval) — set at session creation
  2. Interactive TUI — set mid-session at pkg/cli/runner.go:168 when a user presses "A" (approve all)

So yes, a dedicated sess.NonInteractive flag is the right call. It should be set only in the truly non-interactive session creation paths:

  • pkg/mcp/server.go (MCP server)
  • pkg/a2a/adapter.go (A2A adapter)
  • pkg/evaluation/save.go (eval framework)

I agree the practical impact today is limited — an interactive user who hit max iterations would just lose the ability to extend by 10 more iterations rather than crash — but it's worth fixing properly since that extend-and-continue flow is the designed UX for interactive sessions.

@tdabasinskas would you mind updating the PR with the NonInteractive approach? Should be pretty contained — new field + option on Session, set it in those three places, and swap the check in loop.go. Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants